Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Consolidate validation for 'docvalue_fields'. #59473

Merged
merged 1 commit into from
Jul 22, 2020
Merged

Consolidate validation for 'docvalue_fields'. #59473

merged 1 commit into from
Jul 22, 2020

Conversation

jtibshirani
Copy link
Contributor

@jtibshirani jtibshirani commented Jul 13, 2020 •
edited
Loading

This improves modularity and also fixes some issues when docvalue_fields is
used within inner_hits or the top_hits agg:

  • We previously didn't resolve wildcards in field names.
  • We also forgot to enforce the limit index.max_docvalue_fields_search.

@jtibshirani
Consolidate logic around doc values loading.
fc2521e 
This improves modularity and also fixes some issues when `docvalues_fields` is
used within `inner_hits` or the `top_hits` agg:
* We previously didn't resolve wildcards in field names.
* We also forgot to enforce the limit `index.max_docvalue_fields_search`.
@jtibshirani jtibshirani added >bug :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.9.0 labels Jul 13, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-search (:Search/Search)

@elasticmachine elasticmachine added the Team:Search Meta label for search team label Jul 13, 2020
@jtibshirani jtibshirani changed the title Consolidate validation for 'docvalues_fields'. Consolidate validation for 'docvalue_fields'. Jul 13, 2020
cbuescher
cbuescher approved these changes Jul 17, 2020
View reviewed changes
Copy link
Member

@cbuescher cbuescher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jtibshirani
Copy link
Contributor Author

Thanks @cbuescher for the review. This is technically breaking, since we now enforce the index.max_docvalue_fields_search limit in places where we didn't used to. What's your thought on whether this should be backported or not? I will push a note to the breaking changes docs before merging.

@cbuescher
Copy link
Member

we now enforce the index.max_docvalue_fields_search limit in places where we didn't used to.

As far as I understand the PR this would be limited to using "doc_values" in Top Hits Aggregation and inner hits queries? And the now enforced 100 field limit would probably only be hit when using some sort of wildcard there and there are a huge number of fields?
If there are that many fields, chances are the user needed to update the index.max_docvalue_fields_search already für regular searches where the limit already applies, in which case the limit might already be high enough. And the fact that the limit is changeable dynamically on all indices should make solving this issue in case you run into it also a manageable task.

As far as I'm concerned this should already have triggered an error (I think we fact that we didn't check the limit until now can be considered a bug), so I wouldn't be opposed to backporting to 7.9, maybe adding a note to the changes doc for the cautious. I did a bit of searching in the forums and other support resources where users so far ran into the existing limit, one was in SQL but should be fixed (#44062), and the main problem seems to be this open Kibana issue (elastic/kibana#22897), but that one is concerned with the limit we already check. What's your thoughts on this?

@jtibshirani
Copy link
Contributor Author

jtibshirani commented Jul 22, 2020 •
edited
Loading

And the now enforced 100 field limit would probably only be hit when using some sort of wildcard there and there are a huge number of fields?

This is a good point, it's unlikely that a user would have exceeded the limit if wildcards weren't even supported. I also feel like it makes sense to treat this as a 'bug' and backport to 7.9. (Update: this actually turned out to be 7.10).

@jtibshirani jtibshirani merged commit 6aee768 into elastic:master Jul 22, 2020
@jtibshirani jtibshirani deleted the fetch-doc-values branch July 22, 2020 15:55
@jtibshirani jtibshirani mentioned this pull request Jul 22, 2020
Merged
jtibshirani added a commit to jtibshirani/elasticsearch that referenced this pull request Jul 22, 2020
@jtibshirani
Consolidate validation for 'docvalue_fields'. (elastic#59473)
639b736 
This improves modularity and also fixes some issues when `docvalues_fields` is
used within `inner_hits` or the `top_hits` agg:
* We previously didn't resolve wildcards in field names.
* We also forgot to enforce the limit `index.max_docvalue_fields_search`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cbuescher cbuescher cbuescher approved these changes

Assignees
No one assigned
Labels
>bug :Search/Search Search-related issues that do not fall into other categories Team:Search Meta label for search team v7.10.0 v8.0.0-alpha1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jtibshirani @elasticmachine @cbuescher @jakelandis